Skip to content

stream: fix writev unhandled rejection in fromWeb#62297

Open
Han5991 wants to merge 1 commit intonodejs:mainfrom
Han5991:fix/webstreams-duplex-writev-unhandled-rejection
Open

stream: fix writev unhandled rejection in fromWeb#62297
Han5991 wants to merge 1 commit intonodejs:mainfrom
Han5991:fix/webstreams-duplex-writev-unhandled-rejection

Conversation

@Han5991
Copy link
Copy Markdown
Contributor

@Han5991 Han5991 commented Mar 17, 2026

Summary

Fixes #62199

When using Duplex.fromWeb() or Writable.fromWeb() with cork()/uncork(), writes are batched into _writev(). If destroy() is called in the same microtask (after uncork()), the underlying WritableStream writer gets aborted, causing SafePromiseAll() to reject with a non-array value (e.g. an AbortError or null).

The done() callback in _writev() of both newStreamDuplexFromReadableWritablePair and newStreamWritableFromWritableStream unconditionally called error.filter(), assuming the value was always an array from SafePromiseAll. This caused a TypeError: Cannot read properties of null (reading 'filter') that became an unhandled rejection, crashing the process.

Root cause

done was used as both the resolve and reject handler for SafePromiseAll:

PromisePrototypeThen(SafePromiseAll(chunks, ...), done, done);
  • Resolve path: done([undefined, undefined, ...]) — array, .filter() works
  • Reject path: done(abortError) or done(null) — non-array, .filter() throws

Fix

Separate the resolve and reject handlers of SafePromiseAll. Promise.all resolving means all writes succeeded — there is no error to report. Promise.all rejecting passes a single error value directly.

PromisePrototypeThen(SafePromiseAll(chunks, ...), () => done(), done);
  • () => done() — resolve path: all writes succeeded, call callback with no error
  • done — reject path: single error passed directly to callback

The same fix is applied to both newStreamDuplexFromReadableWritablePair and newStreamWritableFromWritableStream.

Test

Added test/parallel/test-webstreams-duplex-fromweb-writev-unhandled-rejection.js with the exact reproduction from the issue report (cork → write → write → uncork → destroy).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Mar 17, 2026
@Renegade334
Copy link
Copy Markdown
Member

#62291 is already open (although I believe the approach here is the correct one)

@Han5991
Copy link
Copy Markdown
Contributor Author

Han5991 commented Mar 17, 2026

Agreed — and since #62291 is already open, I've opened this as a separate issue because I believe the approach here is the correct one. @Renegade334

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (f48ac91) to head (0a0fe05).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #62297   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         697      697           
  Lines      215749   215747    -2     
  Branches    41304    41289   -15     
=======================================
+ Hits       193681   193682    +1     
  Misses      14161    14161           
+ Partials     7907     7904    -3     
Files with missing lines Coverage Δ
lib/internal/webstreams/adapters.js 86.60% <100.00%> (+0.15%) ⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Han5991
Copy link
Copy Markdown
Contributor Author

Han5991 commented Apr 4, 2026

@Renegade334

Could you possibly review this PR once again? I am aware that there is a previous PR, but it seems quite some time has passed.

@Han5991 Han5991 force-pushed the fix/webstreams-duplex-writev-unhandled-rejection branch 2 times, most recently from 7ce5532 to 5df0476 Compare April 4, 2026 10:13
@Renegade334 Renegade334 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@Han5991
Copy link
Copy Markdown
Contributor Author

Han5991 commented Apr 6, 2026

@Renegade334

#62297 Jenkins failures appear unrelated to this PR.

The failing test across platforms is test-inspector-dom-storage, and the stack trace points to node:internal/inspector/webstorage / node:inspector, not lib/internal/webstreams/adapters.js. The failure matches a separate main regression introduced by d93935b (inspector: auto collect webstorage data, PR #62145), where DOMStorage events were emitted without coercing key/value to strings. That regression was then fixed by 449a93a (inspector: coerce key and value to string in webstorage events, PR #62616).

This Jenkins run appears to have picked up the broken main state before the follow-up fix landed, rather than a problem caused by #62297. Could CI be re-run on the current base?

When using Duplex.fromWeb() or Writable.fromWeb() with cork()/uncork(),
writes are batched into _writev(). If destroy() is called in the same
microtask, the underlying WritableStream writer gets aborted, causing
SafePromiseAll() to reject with a non-array value (e.g. an AbortError).

The done() callback in _writev() of both fromWeb adapter functions
unconditionally called error.filter(), assuming the value was always an
array. This caused a TypeError that became an unhandled rejection,
crashing the process.

Fix by separating the resolve and reject handlers of SafePromiseAll:
use () => done() on the resolve path (all writes succeeded, no error)
and done on the reject path (error passed directly to callback).

Fixes: nodejs#62199
Signed-off-by: sangwook <rewq5991@gmail.com>
@Renegade334 Renegade334 force-pushed the fix/webstreams-duplex-writev-unhandled-rejection branch from 5df0476 to 0a0fe05 Compare April 7, 2026 01:59
@Renegade334 Renegade334 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplex.fromWeb leads to rare internal unhandled rejection

3 participants